feat: port fs related tools from VSC. #894

Hweinstock · 2025-03-31T20:04:40Z

Problem

Some agent tools were implemented in the VSC codebase. However, agentic chat is moving to use Flare implementation, so these tools should live here instead.

relevant: aws/language-server-runtimes#393

Solution

Port fsRead and fsWrite.
Ported from:
- https://github.com/aws/aws-toolkit-vscode/blob/10bb1c7dc45f128df14d749d95905c0e9b808096/packages/core/src/codewhispererChat/tools/fsWrite.ts#L42
- https://github.com/aws/aws-toolkit-vscode/blob/10bb1c7dc45f128df14d749d95905c0e9b808096/packages/core/src/codewhispererChat/tools/fsRead.ts#L17
refactor error handling to allow errors to bubble up.

Testing

Avoid mocking the file system by adding a new test utility TestFolder. This is a simplified version of a this utility from the VSC codebase.

Future Work

Update runtime dependency to ^0.2.49, and add the tools to the agentic chat server here
Does it make sense to move testing utilities to runtimes? Should a non-mocked fs workspace be included?

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

nkomonen-amazon · 2025-03-31T20:51:50Z

@Hweinstock

I get you were just porting, but any idea why we have separate fsRead/Write files? I would have assumed it is better to have a single module with all FS related utils. In this case a wrapper over the basic fs module, but with some more complex utilies.

Avoid mocking the file system by adding a new test utility TestFolder

Mocking the filesystem for tests seems like a code smell, there isn't a strong reason to not just test against the filesystem. So I'm in agreement with your decision to not mock the filesystem and just have them use the TestFolder util.

Does it make sense to move testing utilities to runtimes

What do you mean by this?

nkomonen-amazon · 2025-03-31T20:47:41Z

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/fsRead.ts

+                throw new Error(`Path: "${this.fsPath}" does not exist or cannot be accessed.`)
+            }
+        } catch (err) {
+            throw new Error(`Path: "${this.fsPath}" does not exist or cannot be accessed. (${err})`)


Where does workspace.fs come from? Assuming it is well written, it will already have a usable error code and message. So something like this seems redundant.

My understanding is that this is injected depending on the environment. For the common (non-web) use case, I believe its implemented here: https://github.com/aws/language-server-runtimes/blob/b31f851d7f16d290f29c93281d6646b50820b7fb/runtimes/runtimes/standalone.ts#L174 which doesn't have any error reporting or logging beyond the build in nodefs.

Correct. This is also why you get the linting errors. We try to avoid using Node platform specific filesystem libraries to make portability easier. If that is not possible in your use case, let's see if we can update workspace.fs to meet your requirements, or relax the linting rules for these specific tools.

Just know that if we do the latter, these tools will not be portable from one environment to the next, and that needs to be documented somewhere.

I refactored it to avoid using these modules. I believe that relaxing the lint rules for specific tools could be problematic because it ignores indirect dependencies. For example, we use sanitize from @aws/lsp-core which requires these modules. It does have a note at the top of the file, but lmk if I can document this elsewhere as well.

nkomonen-amazon · 2025-03-31T20:55:23Z

core/aws-lsp-core/src/test/testFolder.ts

+    }
+
+    static async create() {
+        const tempDir = path.join(


nit: A future optimization you could do is delete the root folder at the start/end of the entire test run (if easily doable). This will handle any mistakes from tests failing to clean up properly.

Yes, I agree. I noticed the VS Code implementation does this, and it would be a great feature. We do this by wrapping all the tests with our own test runner in VSC, but I don't see similar logic here yet, so this change would require some ground work.

Hweinstock · 2025-03-31T21:14:22Z

any idea why we have separate fsRead/Write files? I would have assumed it is better to have a single module with all FS related utils. In this case a wrapper over the basic fs module, but with some more complex utilities.

Not entirely sure why they made this design decision. My best guess is that they want to enforce the paradigm of each tool having a singular purpose. Separating it also seems like an easy way to manage granular access to the filesystem by only adding certain tools to an agent.

Mocking the filesystem for tests seems like a code smell, there isn't a strong reason to not just test against the filesystem. So I'm in agreement with your decision to not mock the filesystem and just have them use the TestFolder util.

I agree. The number of filesystem quirks we've observed in VSC shows how easily bugs can slip through (especially on windows).

What do you mean by this?

Added a link to the description. I didn't see any testing utilities other than this testing folder in runtimes, so I am unsure if thats where this stuff should live.

jpinkney-aws · 2025-04-01T12:24:05Z

any idea why we have separate fsRead/Write files? I would have assumed it is better to have a single module with all FS related utils. In this case a wrapper over the basic fs module, but with some more complex utilities.

Not entirely sure why they made this design decision. My best guess is that they want to enforce the paradigm of each tool having a singular purpose. Separating it also seems like an easy way to manage granular access to the filesystem by only adding certain tools to an agent.

I believe its because agentic chat knows how to use tools is by looking up descriptions in the the tool_index.json. The description/input for fsRead/fsWrite are different, so it would be harder to merge them together

jpinkney-aws

I'd probably also call out what commit you copied from in the description just so its easy to find later if you're looking at PR's

kmile · 2025-04-01T13:15:55Z

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/fsRead.ts

+        this.logging.log(`Validation succeeded for path: ${this.fsPath}`)
+    }
+
+    public queueDescription(updates: Writable): void {


Is this meant to update the chat panel with progress on the tool execution?

It currently does not, but that seems like a useful feature. Original implementation lives here. I see an example of how this could be done in Flare here, which would require changes to the runtime to add something like this for file read progress. If that sounds like a good example to base this on I can mark it as a follow-up to add this for file read and write progress.

kmile · 2025-04-01T13:16:47Z

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/fsRead.ts

+    private readonly logging: Features['logging']
+    private readonly workspace: Features['workspace']
+
+    constructor(features: Pick<Features, 'workspace' | 'logging'>, params: FsReadParams) {


Why Pick, is there a reason to scope down the parameter type here?

My intention was require as little as possible from the caller. However, this does force them to explicitly scope Features down so I changed it to Pick<Features, 'workspace' | 'logging'> & Partial<Features> to avoid this step.

That makes sense. This would work or passing in the Feature that you need explicitly. Glad you're not taking a dependency on the whole Features set to avoid coupling everything together.

In order to implement the tool invocation I think you can remove the params here, since in the Agent feature the tool creation is separate from the invocation: https://github.com/aws/language-server-runtimes/tree/main/runtimes#agent-tools

That can happen once we integrate the tools into qChatServer.ts.

justinmk3 · 2025-04-01T14:45:10Z

core/aws-lsp-core/src/index.ts

 export * as timeoutUtils from './util/timeoutUtils'
 export * from './util/awsError'
 export * from './base/index'
+export * from './test/testFolder'


Is it not likely that all these non-aliased * exports will eventually conflict? And that would then require quasi-namespacing. Instead of quasi-namespacing, we can solve the problem easily by aliasing, like this:

Suggested change

export * from './test/testFolder'

export * as testFolder from './test/testFolder'

btw is it intentional/unavoidable that test/testFolder is part of the core (non-text) index.ts ?

I did not see an existing location for test utilities, and this seemed like the most reasonable. The closest thing I saw was this in runtimes, but having to bump runtimes version every time you edit a test utility seemed excessive. If there is a more appropriate place to put this lmk.

Also, I imagine that it is unlikely TestFolder is reused, but I understand this is good practice going forward.

justinmk3 · 2025-04-01T14:46:51Z

core/aws-lsp-core/src/test/testFolder.ts

+
+/**
+ * Interface for working with temporary files.
+ * Simplified port of https://github.com/aws/aws-toolkit-vscode/blob/16477869525fb79f8dc82cb22e301aaea9c5e0c6/packages/core/src/test/testUtil.ts#L77


👍 for now, this kind of precise cross-reference will help a lot, in each of these util files.

justinmk3 · 2025-04-01T15:38:39Z

core/aws-lsp-core/src/util/path.ts

    return normalizeSeparator(path.normalize(p))
 }
+
+export function sanitize(inputPath: string): string {


In aws-toolkit-vscode this santize() was added recently, but I don't get why it was added: aws/aws-toolkit-vscode#6840 (comment)

Why is this needed instead of our normalize() ?

It looks like they do slightly different things that definitely overlap with normalize. For example, normalize doesn't remove white space or expand ~. I agree they definitely overlap and could potentially be refactored to make this more clear.

imykhai · 2025-04-02T06:28:00Z

One of the commits - a merge from main to the branch - doesn't comply with conventional commits.

Hweinstock · 2025-04-02T13:35:02Z

I was planning to squash-merge since the individual commits are not important. Lmk if rebasing is preferred.

Hweinstock added 7 commits March 31, 2025 14:48

feat: port fsRead with minimal changes

1a45400

refactor: move testFolder to a general location

c7692fa

feat: port over fsWrite with minimal changes

f70f694

refactor: clean up testFolder util

0d8f7b9

docs: add links to original implementations

706747b

refactor: clean up comments

f441d4d

fix: properly import crypto

8febafb

Hweinstock marked this pull request as ready for review March 31, 2025 20:33

Hweinstock requested a review from a team as a code owner March 31, 2025 20:33

nkomonen-amazon approved these changes Mar 31, 2025

View reviewed changes

jpinkney-aws approved these changes Apr 1, 2025

View reviewed changes

kmile reviewed Apr 1, 2025

View reviewed changes

Hweinstock added 2 commits April 1, 2025 09:57

refactor: avoid explicit scope down of features

108f8d4

fix: lint rules

9f72fc5

Hweinstock force-pushed the agentic-chat/fs branch from ace27f7 to 9f72fc5 Compare April 1, 2025 14:31

justinmk3 reviewed Apr 1, 2025

View reviewed changes

refactor: name export in index.ts

49f47b1

Hweinstock force-pushed the agentic-chat/fs branch from 7d7ccb1 to 49f47b1 Compare April 1, 2025 15:20

justinmk3 reviewed Apr 1, 2025

View reviewed changes

Hweinstock added 6 commits April 1, 2025 13:16

refactor: pass params at invoke time

216db07

Merge branch 'main' into agentic-chat/fs

bfc96a7

feat: add fs tools server

3b7bf15

refactor: remove queue description

a9b8804

test: port path tests related to sanitize

a430ad3

refactor: avoid unnecessary export

1949a56

refactor: validate before invoking

4c02f9f

Hweinstock force-pushed the agentic-chat/fs branch from c7f70ae to 4c02f9f Compare April 1, 2025 21:04

Hweinstock added 2 commits April 1, 2025 18:04

refactor: avoid excessive rethrowing

235affc

refactor: explicitly type input

ceebf24

imykhai approved these changes Apr 2, 2025

View reviewed changes

Hweinstock added 2 commits April 2, 2025 09:17

Merge branch 'main' into agentic-chat/fs

eb820bc

refactor: make spec readonly

af5c752

fix: avoid unnecessary type assertion

b3aa351

nkomonen-amazon approved these changes Apr 2, 2025

View reviewed changes

Hweinstock merged commit a368acc into aws:main Apr 3, 2025
6 checks passed

Hweinstock deleted the agentic-chat/fs branch April 3, 2025 15:46

github-actions bot mentioned this pull request Apr 3, 2025

chore(release): release packages from branch main #898

Merged

This was referenced Apr 9, 2025

chore: sync fsRead with VSC branch. #936

Merged

test: expand fsWrite test suite and sync with VSC #937

Merged

	export * from './test/testFolder'
	export * as testFolder from './test/testFolder'

feat: port fs related tools from VSC. #894

feat: port fs related tools from VSC. #894

Uh oh!

Conversation

Hweinstock commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Testing

Future Work

License

Uh oh!

nkomonen-amazon commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hweinstock Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hweinstock Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hweinstock commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpinkney-aws commented Apr 1, 2025

Uh oh!

jpinkney-aws left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hweinstock Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmile Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk3 Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imykhai commented Apr 2, 2025

Uh oh!

Hweinstock commented Apr 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Hweinstock commented Mar 31, 2025 •

edited

Loading

nkomonen-amazon commented Mar 31, 2025 •

edited

Loading

Hweinstock Mar 31, 2025 •

edited

Loading

Hweinstock Apr 1, 2025 •

edited

Loading

Hweinstock commented Mar 31, 2025 •

edited

Loading

jpinkney-aws left a comment •

edited

Loading

Hweinstock Apr 1, 2025 •

edited

Loading

kmile Apr 1, 2025 •

edited

Loading

justinmk3 Apr 1, 2025 •

edited

Loading